Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stlite modifications #672

Merged

Conversation

lukasmasuch
Copy link
Contributor

@lukasmasuch lukasmasuch commented Jan 8, 2024

Related pull request with Streamlit modifications: whitphx/streamlit#7

@lukasmasuch lukasmasuch changed the title [WIP] Add stlite modifications Add stlite modifications Jan 9, 2024
Copy link
Owner

@whitphx whitphx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!
Let me comment as below at this moment, while I'm gonna check the code more deeply soon.

@@ -281,7 +280,6 @@ async function loadPyodideAndPackages() {
// The following Python code is based on streamlit.web.cli.main_run().
self.__streamlitFlagOptions__ = {
...streamlitConfig,
"browser.gatherUsageStats": false,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this feature should be enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. From our data side, we are able to handle and distinguish telemetry coming from stlite apps. And in the long-term if stlite gets more integrated as an official Streamlit aspect, we probably want to enable it for all stlite apps. But for this update, I think its fine to leave it disabled as default but in a way that it can be activated if it is explicitly set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the update code

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

packages/common-react/tsconfig.json Show resolved Hide resolved
packages/kernel/py/stlite-server/stlite_server/server.py Outdated Show resolved Hide resolved
packages/kernel/py/stlite-server/stlite_server/server.py Outdated Show resolved Hide resolved
widget_id = self._require_arg(args, "widgetId")
path_args = re.match(UPLOAD_FILE_ROUTE, request.path)
session_id = path_args.group('session_id')
file_id = path_args.group('file_id')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Check if session_id and file_id can be obtained from kwargs of this handler?
(Now I'm outside and can't confirm it, but think maybe it's possible)

@@ -1,5 +1,4 @@
export * from "./kernel";
export * from "./streamlit-replacements/lib/ConnectionManager";
export * from "./streamlit-replacements/lib/FileUploadClient";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memo: The patched FileUploadClient has been moved to whitphx/streamlit in whitphx/streamlit#7

Copy link
Owner

@whitphx whitphx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and it LGTM!
I'm gonna merge this PR, and let me add some modifications in #671 after merging.

@@ -51,6 +51,7 @@ export interface WorkerInitialData {
};
mountedSitePackagesSnapshotFilePath?: string;
streamlitConfig?: StreamlitConfig;
disableProgressToasts?: boolean;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed here, as the worker doesn't refer to this option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes that makes sense 👍

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I updated it as a part of changes in #671 after merging this PR. Plz track it and leave any comment 👍

@whitphx whitphx merged commit 42f3a4a into whitphx:dev/upgrade-streamlit-1.27.0 Jan 17, 2024
whitphx added a commit that referenced this pull request Jan 19, 2024
* Copy .nvmrc from [email protected]

* Update the streamlit submodule, fix some package resolutions for it, and fix @stlite/kernel for streamlit 1.27.0

* Update @stlite/common-react and mountable

* Set webpack version in @stlite/mountable as same as [email protected]

* Set the resolved webpack version to avoid an error "The 'compilation' argument must be an instance of Compilation", which is caused by multiple versions of webpack. Ref: https://qiita.com/Hanpen3019/items/e6ae5d29b334bc3d7af3

* Update @stlire/sharing

* Update @stlite/desktop

* Update Makefile

* Remove blob-polyfill

* Update Makefile

* Update the streamlit submodule

* Fix .github/workflows/main.yml

* Add streamlit/frontend as a workspace

* Add a make rule for the streamlit frontend lib

* Update wheel versions

* Update the streamlit submodule

* Set the immutable's version as same as the locked version of the upstream Streamlit frontend to avoid type errors

* Add stlite modifications (#672)

* Add stlite modifications

* Add back connection manager

* Move methods

* Some updates

* Remove header

* More cleanup

* More changes

* More cleanup

* More modifications

* Don't update pyiodide

* More cleanup

* Don't Update pyodide

* Don't update pyiodide

* More cleanup

* More cleanup

* Rename of option

* Remove new line

* Minor cleanup

* Fix wrong option name

* Minor renaming

* Fix typo

Co-authored-by: Yuichiro Tachibana (Tsuchiya) <[email protected]>

* Update comment

Co-authored-by: Yuichiro Tachibana (Tsuchiya) <[email protected]>

* Remove paths

* Change to put

* Update URL

* Add gatherUsageStats flag

---------

Co-authored-by: Yuichiro Tachibana (Tsuchiya) <[email protected]>

* Update the streamlit submodule

* Add the PUT type to the HttpRequest interface

* Fix the imports in ConnectionManager.ts

* Fix upload_file_request_handler.py to refer to `kwargs` of the handler method

* Fix stlite_server/upload_file_request_handler.py to catch up with the upstream implementation

* Remove `form-data-encoder` from `@stlite/kernel` as it's moved to `@streamlit/lib` in whitphx/streamlit#7

* Fix the mountable option parser to distinguish the toast callback options from the kernel options

* Remove ctx.disableToast() and ctx.enableToast() which were introduced in #636 because the `disableProgressToasts` and `disableErrorToasts` options play the same role

* Update CHANGELOG.md

* Apply Prettier to kernel

* Fix stlite-server tests

* Fix the CI workflows

* Fix format and types in stlite-server

* Update main.yml fixing the build jobs to initialize with venv

* Add the streamlit lib package to the deps of the kernel make rule

* Remove the TS dependency from the kernel module to the build artifact of `@streamlit/lib`

* Fix main.yml

* Fix .github/workflows/gh-pages.yml

* Update the streamlit submodule

* Fix the streamlit-proto make rule to call the python-init-dev-only rule

* Fix the venv make rule to execute it every time

* Update DEVELOPMENT.md

* Fix desktop/bin/dump_artifacts.ts not to pin the altair version

* Fix CI files

* Fix tsconfig

* Refresh package.json and yarn.lock

* Fix stlite_server/server.py making it look more similar to the original server.py

* Fix dump_artifacts.ts

* Add a test for the file delete endpoint

---------

Co-authored-by: Lukas Masuch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants